Lambda support in VM: Wolfgang's PR plus some simplifications (removing mask, clarifying ops) #15387
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
22 tasks
⏱️ 1h 30m total CI duration on this PR
🚨 1 job on the last run was significantly faster/slower than expected
|
This was referenced Nov 25, 2024
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
brmataptos
changed the title
brmataptos
changed the title
brmataptos
requested review from
banool,
gregnazario and
0xmaayan
as code owners
brmataptos
requested review from
georgemitenkov,
runtian-zhou,
vgao1996 and
wrwg
| // Since bytecode version 8 | ||
| LdFunction(_) => Opcodes::LD_FUNCTION, | ||
| LdFunctionGeneric(_) => Opcodes::LD_FUNCTION_GENERIC, | ||
| Invoke(_) => Opcodes::INVOKE, |
There was a problem hiding this comment.
Probably we want InvokeFunction, EarlyBindFunction for consistency?
There was a problem hiding this comment.
Done.
Though I wonder if we should rename Call as CallFunctionHandle for similar consistency. :-)
16 tasks
brmataptos
force-pushed
the
11-21-lambda-compiler-completion
branch
from
42421fe
to
92ae56c
Compare
brmataptos
force-pushed
the
11-23-vm_changes_merged_in
branch
from
ebb0fcc
to
8154bb7
Compare
brmataptos
force-pushed
the
11-21-lambda-compiler-completion
branch
from
92ae56c
to
3c02ba1
Compare
brmataptos
force-pushed
the
11-23-vm_changes_merged_in
branch
from
d52d75a
to
f49f228
Compare
rahxephon89
reviewed
Nov 30, 2024
| ] | ||
| } | ||
|
|
||
| pub static INITIAL_COST_SCHEDULE: Lazy<CostTable> = Lazy::new(|| { | ||
| let mut instrs = bytecode_instruction_costs(); | ||
| // Note that the DiemVM is expecting the table sorted by instruction order. |
There was a problem hiding this comment.
Could you give more info on why this is removed?
There was a problem hiding this comment.
Note that new_from_instructions sorts the parameter the same way, so no need to sort again here.
brmataptos
force-pushed
the
11-21-lambda-compiler-completion
branch
from
3c02ba1
to
ae4444b
Compare
brmataptos
force-pushed
the
11-23-vm_changes_merged_in
branch
from
f49f228
to
ab7e327
Compare
[move-vm] Types and opcodes for closures This PR implements the extensions needed in the file format for representing closures: - The type (SignatureToken) has a new variant `Function(args, result, abilities)` to represent a function type - The opcodes are extendeed by operations `ClosPack`, `ClosPackGeneric`, and `ClosEval` This supports bit masks for specifyinng which arguments of a function are captured when creating a closure. Bytecode verification is extended to support the new types and opcodes. The implementation of consistency, type, and reference safety should be complete. What is missing are: - File format serialization - Interpreter and value representation - Value serialization - Connection of the new types with the various other type representations
…rser work: - `LdFunction`, `LdFunctionGeneric` = generate a closure from a defined function - `EarlyBind` = bind more arguments to any closure - `Invoke` = call a closure with final arguments Add some description/semantics to `file_format.rs` to better describe the implementation which will be needed. Get rid of complex Mask calculations in favor of simpler early bninding of `k` initial arguments. Hopefully keep all bytecode verifier operations working the same.
…eFunction based on George's suggestion
brmataptos
force-pushed
the
11-21-lambda-compiler-completion
branch
from
ae4444b
to
8b37927
Compare
brmataptos
force-pushed
the
11-23-vm_changes_merged_in
branch
from
ab7e327
to
84b10f6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR started with a cherry-pick commit of Wolfgang's draft PR 15171 = commit #d85b919.
On top of that, I
not requiring a defined function handle except in
LdFunction[Generic].EarlyBindandInvokebecause of the way the verifier works today.file_format.rswhich I'd worked out previously.The current operations are:
-
LdFunction,LdFunctionGeneric= generate a closure from a defined function-
EarlyBind= bind more arguments to any closure-
Invoke= call a closure with final argumentsAs noted by Wolfgang in his commit, we are still lacking:
How Has This Been Tested?
Barely tested, just running through existing tests
Key Areas to Review
Did I lose any verifier support when modfiying Wolfgang's opcodes?